feat: Add client validation for structuredContent #302
Conversation
mcp-test/src/main/java/io/modelcontextprotocol/server/.LCKAbstractMcpSyncServerTests.java~
Outdated
Show resolved
Hide resolved
mcp/pom.xml
Outdated
| <!-- Json validator dependency --> | ||
| <dependency> | ||
| <groupId>com.networknt</groupId> | ||
| <artifactId>json-schema-validator</artifactId> | ||
| <version>1.5.7</version> | ||
| </dependency> |
There was a problem hiding this comment.
Worth getting extra clarification from maintainers on if we want schema validation as part of this or a separate effort, along the lines of the discussion in #271.
There was a problem hiding this comment.
Discussed some of the issues offline with @LucaButBoring. I wanted to get clarity around the scope of the work on the issue that I had created. There are a few questions that we might want to answer regarding the output validation:
- Do we want validation to be part of this PR or do we want to separate it out and think more about how we can cache/validate the results?
- Should cache be in the client class or should we construct a separate cache class?
- Should there be refresh intervals for cache invalidation? How else can we deal with it and should it be configurable by the client?
- Is a Map the best way to represent cache or should we opt for an in-memory cache implementation like Guava Cache?
Would like inputs from the maintainers to decide on the best way to proceed.
mcp/src/main/java/io/modelcontextprotocol/client/McpAsyncClient.java
Outdated
Show resolved
Hide resolved
mcp/src/main/java/io/modelcontextprotocol/client/McpSyncClient.java
Outdated
Show resolved
Hide resolved
| catch (JsonProcessingException e) { | ||
| // Log error if output schema can't be parsed to prevent erroring out for | ||
| // successful call tool request | ||
| logger.error("Encountered exception when parsing outputSchema: {}", e); | ||
| } |
There was a problem hiding this comment.
Worth getting other opinions on this, I think this should be rethrown (potentially wrapped in McpError to keep it unchecked).
There was a problem hiding this comment.
I agree. For now, changing this to log a warning. This leaves the option open for the client side implementation to proceed as it deems fit. I would like feedback from others about what would be the ideal behavior in this scenario.
mcp/src/main/java/io/modelcontextprotocol/client/McpSyncClient.java
Outdated
Show resolved
Hide resolved
|
Just gave this a preliminary review by request, review done for now. |
|
Addressed all comments from @LucaButBoring barring a few that where I would like a discussion with maintainers/others from the community. |
| @@ -215,7 +244,54 @@ public Object ping() { | |||
| * Boolean indicating if the execution failed (true) or succeeded (false/absent) | |||
| */ | |||
| public McpSchema.CallToolResult callTool(McpSchema.CallToolRequest callToolRequest) { | |||
| return this.delegate.callTool(callToolRequest).block(); | |||
| McpSchema.CallToolResult result = this.delegate.callTool(callToolRequest).block(); | |||
There was a problem hiding this comment.
Note: Shift to sync instead.
pantanurag555
force-pushed
the
output-schema
branch
from
f088e75 to
1911386
Compare
pantanurag555
changed the title
|
Updated the PR to leverage the existing json validator and to perform only client-side validations. Added a new test file to mock transport for the sync client. |
Motivation and Context
This PR adds client-side validation for CallToolResult.structuredContent. The specification was introduced in modelcontextprotocol/modelcontextprotocol@03dc24b.
Resolves #285
Changes in this PR:
How Has This Been Tested?
Breaking Changes
None
Types of changes
Checklist